-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated mono testing doc #44360
Updated mono testing doc #44360
Conversation
Tagging subscribers to this area: @CoffeeFlux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we already have a file for running tests on Mono at https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/mono/testing.md
We should be cleaning up/extending this instead.
The file name is a little bit misleading. This file is for running runtime tests only. I have another PR for that which refers to this file, feel free to review that as well. |
Is there a reason we're moving some of those instructions over to the CoreCLR directory? It seems better suited in the original document. |
Follow the fashion as this doc: |
I think that right now we're better off avoiding splitting mono instructions into a directory that's otherwise CoreCLR-specific (and out of date). It seems confusing and unlikely to be found there. If we restructure the docs to have a runtime tests folder (as opposed to a CoreCLR one) I think it would make more sense, but otherwise I'd prefer to preserve the current structure. |
It makes more sense to keep the detailed instructions of running runtime tests with mono together with other runtime tests running instructions. Because there are other details about the runtime tests which are not covered by this doc and could easily be discovered, if they all live in the same folder. |
I think the primary use of the docs is people looking for quick instructions on "how to do [x]", and this doesn't strike me as conducive to that. Again, I'd be fine with a larger restructuring to group everything by test suite, but I'd rather avoid mixing approaches since that makes docs unnecessarily difficult to find. At the very least, it would be nice to get some other opinions before merging a structural change like this, so I'm going to leave it blocked for now. I'd also prefer to group that other PR with this one since they're dependent on each other; I don't think it makes sense to review or merge them separately. |
@fanyang-mono - For libraries, since now they are common for both the runtimes, it makes sense the way they have it. For Mono and CoreCLR, it makes sense to have individual how-tos documentation in their relative folder structures. It makes more sense to leave Mono instructions in the mono dir. You should update the name of the file 'testing.md' to 'testing-mono.md' and update that file contents. You can close this PR, and use the other PR #44362 to make those changes. @steveisok - It makes sense to extract the testing libraries section from here to under the libraries documentation. Any recommendations ? |
I feel that there maybe a misunderstanding here, which probably was caused by creating two separated PR's for my doc changes. That was the only option which was the only option was given to me when editing online. I will find a way to merge my 2 PR's. This file is NOT a replacement of the original testing doc for mono. This file only contains content regarding to runtime tests. That file still exist and have a direct link to this new file. So this won't make discovering mono documents more difficult. Again, the reason why I put this file here is because other files in this folder covers quite a few details about the nature of runtime tests. The runtime tests are now being shared by coreclr and mono. Maybe this folder name could be changed from That's being said, I am okay with merging the content here to mono/testing.md, if this setup is confusing to most people. My motivation was to make it easy to find information that you need. |
e6f2d1e
to
6a6d421
Compare
Closed #44362 and kept this one, since all the discussions were done here. |
Created another issue (#44433) to centralize the document for runtime tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me as far as the actual documentation goes. I am fine with leaving it in mono/testing.md and merging the two testing document dirs as a seperate issue.
@CoffeeFlux Are you okay with this change now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
I went through a bunch of restructuring specifics in the PR, but for some broader feedback I recommend using more concise and direct wording when writing technical documentation. Some of my suggested edits reflect that, but there are more places where that could be cleaned up.
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
@CoffeeFlux Any other feedback? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting there! Few more stylistic notes, some comments on using more direct and inclusive language, and then a suggestion on how to restructure the samples section.
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple specific things left, but structurally I'm happy with where it's at now. Should be good once you fix those.
|
||
3. Run the tests | ||
* To run the desktop Mono sample, cd to `HelloWorld` and execute: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bullet points are back :(
Please use sub-headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are very short. I thought bullet points were more suitable.
Co-authored-by: Ryan Lucia <[email protected]>
Co-authored-by: Ryan Lucia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
No description provided.